there is no need for the Note: here. this is just another sentence providing the additional context.

are there benefits of having this as a pointer field?

No, just a mistake I copied from somewhere else. Fine to drop the pointer

If the templated string exceeds 63 characters, it will be trimmed to 58 characters and will
get concatenated with a random suffix of length 5.

should there be an explicit test case for this on the MachineNamingStrategy level?
presumably https://github.com/kubernetes-sigs/cluster-api/blob/90c4712f67bf21fce52e07a57488f62497306699/internal/topology/names/names.go#L40C30-L40C43 has tests, but might be worth testing on this level too.

I assume

fixed

I think we should provide also cluster name

Okay for me

I have added clustername too.

Let's add a test case for the default behaviour (no template) and for when the generated name exceeds the lenght

done, following cases are added:

• User-defined template
• template exceeding 63 chars.
• invalid template
• no template

We are already testing some cases here: https://github.com/kubernetes-sigs/cluster-api/blob/main/internal/topology/names/names_test.go

nit

this was not invalid; the test was named wrong and is now fixed.
I have added a new test with an invalid template too.

What happens if the combination of the name and random suffix is 64 characters? Do we then trim and add a new random suffix?

Yes

	if len(name) > maxNameLength {
		name = name[:maxGeneratedNameLength] + utilrand.String(randomLength)
	}

(maxNameLength is 63)

Why expose this rather than always appending -<random 5 chars> at the end of the string?

I see the following reasons:

• We already have namingStrategies in the ClusterClass that work the same way as the one introduced here (so consistency would be nice)
  • In that case we use the namingStrategies to calculate control plane / MD and MP names. In these cases it was desirable to not always end up with a random suffix. E.g. folks wanted to be able to have KCP names that are identical to Cluster names
• Another case would be if folks have the (admittedly strange) use case that the random part shouldn't be at the end of the name (there was one (!) person asking for it in Slack, they wanted a random prefix for some reason, xref: https://kubernetes.slack.com/archives/C8TSNPY4T/p1724389852680579)

Note that bullet points look great here, but the formatting does not transfer well to kubectl explain, review the CRD description of this field and you'll see the formatting is all squashed. Instead, use full sentences to explain the same context.

Something along the lines of

// The template allows the following variables ".kubadmcontrolplane.name", ".something.else" and ".random".
// The variable ".kubeadmcontrolplane.name" retrieves the name of the KubeadmControlPlane object that owns the Machines being created.
// The variable ".random" is substituted with a 5 character alphanumeric string on creation.
// Variables in the template must be enclosed in double braces ("{{ <variable> }}").

done

You should impose a maximal length on this string, you know that the rendered value must not exceed 63 chars, so it needs to be at least that in length, assuming a worst case, the value of any substituted variable could be 1 character right?

So it needs to be 62 plus the length in the template of representing the longest variable. Currently that is {{ .kubeadmcontrolplane.name }} which is 31 characters, so, a limit of 93 would be suitable presently

I think we can do this slightly better and easier. We already have similar validation for the other namingStrategies we have:

So we can verify:

• that the template actually renders
• that the rendered output is a valid name
• use the minimal length for our parameters and then check if the generated output is too long
  • Not sure if we actually ever hit that case, because we have that safeguard that we just cut if rendered string gets too long. So I think we don't need this validation?
  • Note: You can do a bunch of stuff in templates, so I think the template can be a lot longer without actually leading to long rendered values.

Sorry I am a bit confused, do we need restrictions on minimum chars too for the generated name?

@JoelSpeed wdyt based on my comment above?

I think that sounds reasonable yes! So that would be webhook based validation, you should still also include an API side maximum length though (since we can calculate a reasonable maximum)

Having maximum lengths on all fields is good practice and helps with the runtime cost analysis that's built into the CEL libraries, should we ever get to using those.

You can add length limits when the API is introduced, restricting them later is harder

Ah yeah, good point. I forgot the CEL part

The validation webhook that is in clusterclass I believe is checking all the things needed. I have added the same here, let me know if anything needs to be updated.

I can see that the webhook is checking the template, but, I think we do still want to have a length limit as a marker on the API field here, for completeness

I have added the marker for max length please check.

Is it possible to impose a pattern validation on this string?

Is there any admission time validation at present that checks that this compiles and would produce a valid template?

Good catch!
(I didn't review the implementation myself yet, only the API)

We should have a similar validation than what we have for other naming strategies:

I have added the validation webhook, please check.